Sui Lazer contract governance #3316
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Depends on #3317 |
ali-behjati
left a comment
ali-behjati
left a comment
There was a problem hiding this comment.
i reviewed only the sui contract. it seems correct but i think the structure can be improved. There are mutations happening in both state.move and governance.move [on process incoming message] which makes reading hard. if possible, my recommendation is to keep state minimal and move all the business logic affecting state in governance.move file itself.
| # supported, and testnet one is already outdated, so we either want to have the | ||
| # package at MVR at some point, or maybe want to consider creating our own | ||
| # branches with symlinked `Move.toml`. | ||
| rev = "sui/mainnet" |
There was a problem hiding this comment.
the story about this is that they were blocked on some issues with the move package manager handling multiple local packages to upgrade. they haven't fixed it yet
There was a problem hiding this comment.
Hmm, I see 🫤 I'm playing with custom testnet branch for now, but I wonder what branches to use after finishing the PR.
| /// Reference: | ||
| /// https://wormhole.com/docs/products/reference/chain-ids/#__tabbed_1_1 | ||
| /// https://github.com/pyth-network/pyth-crosschain/blob/b021cfe9b2716947f22d1724cd3fa7e3de6b026e/governance/xc_admin/packages/xc_admin_common/src/chains.ts#L274 | ||
| const RECEIVER_CHAIN_ID: u16 = 21; |
There was a problem hiding this comment.
Is it possible to make this a customizable parameter (initialized on package init)? we might deploy to multiple SUI-like networks.
There was a problem hiding this comment.
Sure - question is, given that meta module will already require source change for every individual deploy, whether it wouldn't make sense to put it there instead.
We could even generate meta on demand when building for specific chain and deploy. This way it's one less field to store on-chain.
There was a problem hiding this comment.
I've implemented the latter approach, but let me know if it's better to do differently.
There was a problem hiding this comment.
meta comment: couldn't we put it in governance.move?
There was a problem hiding this comment.
Ok, I will try to come up with a better structuring.
There was a problem hiding this comment.
we might want to use it for parsing payload data as well.
it's not super clear to me what value we get out of this wrapper. but it's fine.
There was a problem hiding this comment.
It's just to decouple our parsing logic from wormhole, so that we can use method syntax and possibly add more methods later. Should I port over payload parsing, or leave it as it is for now?
Just a note - Parser type must be public because of language restrictions, but the module doesn't expose anything else outside of the package, so the API is de facto fully upgradeable. It will just require creating a new parser type as a replacement.
| assert!(self.chain_id == chain_id, EMismatchedEmitterChainID); | ||
| assert!(self.address == address, EMismatchedAddress); | ||
| // TODO: See https://wormhole.com/docs/protocol/infrastructure/vaas/#verified-action-approvals | ||
| // - is this enough to avoid replay attacks? |
There was a problem hiding this comment.
Specifically related to these paragraphs:
The sequence field depends on the final ordering of blocks on the emitter chain. When a lower consistency level is chosen (i.e., not waiting for finality), there is a chance that chain reorganizations could lead to multiple, different VAAs appearing for what looks like the “same” message on the user side.
The tuple (emitter_chain, emitter_address, sequence) can only be considered unique if the chain does not undergo a reorg and the block containing the message has effectively reached finality. However, there is always a small chance of an extended reorg that could invalidate or alter a previously emitted sequence number.
I left the comment here during PR just to address this - do we want to put lower bound on consistency level, or is it relevant at all in case of a Solana emitter?
| /// would give them their current address or version, we track it manually here. | ||
| /// | ||
| /// WARNING: Construction of `CurrentCap` requires this version to match the | ||
| /// `UpgradeCap` version, and thus attempts to publish or upgrade the package |
There was a problem hiding this comment.
how does the upgrade cap version change? does it increment by 1 each time?
There was a problem hiding this comment.
Yes, it should be incremented by one on every upgrade, starting with 1 on initial publish. I will try to automate this to avoid invalid upgrades once I finish the contract manager script.
There was a problem hiding this comment.
See manage_sui_lazer_contract.ts.
| } else if (header.action === "UpdateTrustedSigner") { | ||
| return fc | ||
| .record({ | ||
| publicKey: hexBytesArb({ minLength: 33, maxLength: 33 }), |
There was a problem hiding this comment.
They are supposed to be Bitcoin-style (secp256k1) ECDSA compressed keys, which are 32 bytes plus 1 byte prefix. Same as output of secp256k1_ecrecover during update verification.
There was a problem hiding this comment.
add tests with fixtures for the governance
🚧 in progress 🚧
Summary
Implements support for Pyth governance messages provided as Wormhole VAAs, and exposes actions for initialization, upgrading and trusted signer updates. Updates
xc_adminandcontract_managerto support new types of messages and maintenance actions.The PR adds a new "Lazer" module for governance actions related to Lazer contracts, as these don't map well to existing Core actions. From my understanding, existing Lazer contracts seem to use custom payload for governance, or do not consume governance messages at all, so they don't seem to be affected yet.
Rationale
How has this been tested?
I've updated and run
lazer/contracts/suitest suite and extended (and run)xc_admintest suite to cover new messages.